Skip to content

[security] fix(providers): reject sibling HTTP endpoint overrides - #1256

Merged
steipete merged 5 commits into
steipete:mainfrom
Hinotoi-agent:harden-sibling-provider-endpoint-overrides
Jun 11, 2026
Merged

[security] fix(providers): reject sibling HTTP endpoint overrides#1256
steipete merged 5 commits into
steipete:mainfrom
Hinotoi-agent:harden-sibling-provider-endpoint-overrides

Conversation

@Hinotoi-agent

@Hinotoi-agent Hinotoi-agent commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR hardens sibling provider API endpoint overrides for credentialed usage calls.

  • Rejects explicit non-HTTPS endpoint overrides for OpenRouter, Codebuff, Groq, and ElevenLabs.
  • Preserves the existing convenience behavior where bare host/path override values are normalized to HTTPS.
  • Adds provider-level validation before credentialed usage fetches construct requests.
  • Adds regression coverage for HTTPS acceptance, bare-host normalization, and explicit HTTP rejection.

Security issues covered

Issue Impact Severity
Credentialed provider endpoint overrides accepted explicit HTTP URLs across OpenRouter, Codebuff, Groq, and ElevenLabs A local configuration or environment override could direct provider-owned credentialed API calls to plaintext HTTP, exposing bearer/API-key authenticated traffic to network interception or downgrade paths. Medium

Before this PR

  • OPENROUTER_API_URL, CODEBUFF_API_URL, GROQ_API_URL, and ELEVENLABS_API_URL could be set to explicit http://... URLs.
  • The corresponding usage fetchers built credentialed requests using the configured base URL.
  • The sibling provider paths did not have regression coverage matching the endpoint hardening already added for Alibaba/MiniMax.

After this PR

  • Explicit non-HTTPS endpoint overrides are rejected during provider settings validation.
  • Usage fetchers call the validation path before constructing credentialed requests.
  • Bare host/path overrides still infer HTTPS for compatibility.
  • A shared regression test locks the behavior for OpenRouter, Codebuff, Groq, and ElevenLabs.

Why this matters

These API URL settings are provider-owned credentialed endpoints. Once a token/API key is present, allowing an explicit HTTP override creates an unnecessary downgrade path for authenticated traffic. Rejecting non-HTTPS overrides keeps the secure default while preserving a safe HTTPS override mechanism for trusted local testing or alternate provider-compatible endpoints.

How this differs from related issue/PR

This is a sibling-provider follow-up to #1236. That PR covers Alibaba and MiniMax endpoint overrides. This PR applies the same boundary to OpenRouter, Codebuff, Groq, and ElevenLabs, whose settings readers and usage fetchers are separate code paths.

Attack flow

local configuration/environment sets provider API URL to http://...
    -> provider settings reader accepts the override
        -> usage fetcher builds a credentialed request from that base URL
            -> bearer/API-key authenticated provider traffic can be sent over plaintext HTTP

Root cause

  • API URL override readers normalized or accepted caller-controlled base URLs without rejecting explicit non-HTTPS schemes.
  • Credentialed usage fetchers trusted those base URLs before sending provider authentication headers.
  • Endpoint override validation was inconsistent across providers.

Changes in this PR

  • Adds explicit endpoint override validation to OpenRouter and Codebuff settings errors/descriptors.
  • Adds validateEndpointOverrides(environment:) guards to OpenRouter, Codebuff, Groq, and ElevenLabs credentialed usage fetchers.
  • Keeps HTTPS inference for bare host/path override values.
  • Adds ProviderEndpointOverrideSecurityTests covering accepted HTTPS, bare-host normalization, rejected HTTP fallback, and thrown validation errors.

Maintainer impact

  • Narrow provider settings/fetcher hardening only.
  • No real provider calls or production credentials are required by the new tests.
  • Existing HTTPS endpoint overrides and bare-host override values remain supported.
  • Explicit plaintext HTTP endpoint overrides are now rejected for credentialed provider-owned API calls.

Type of change

  • Security fix
  • Tests
  • Documentation update
  • Refactor with no behavior change

Test plan

  • swift build --target CodexBarCore
  • Focused standalone Swift smoke for OpenRouter/Codebuff/Groq/ElevenLabs endpoint override behavior with fake test values, including host:port bare overrides
  • swiftformat lint mode via make check reported 0/998 files require formatting
  • Direct SwiftLint on the touched files with the local CommandLineTools SourceKit path reported 0 violations
  • swift test --filter ProviderEndpointOverrideSecurityTests — blocked locally by existing KeyboardShortcuts #Preview macro/plugin build failure before these tests run:
    • external macro implementation type 'PreviewsMacros.SwiftUIView' could not be found for macro 'Preview(_:body:)'
  • Full make check — blocked locally after SwiftFormat passes because the wrapper SwiftLint invocation traps while loading SourceKit:
    • Fatal error: Loading sourcekitdInProc.framework/Versions/A/sourcekitdInProc failed

Executed with redacted/fake endpoint values only; no provider services or real credentials were used.

$ swift build --target CodexBarCore
Build of target: 'CodexBarCore' complete! (5.46s)

$ swiftc /tmp/codexbar_endpoint_override_smoke.swift \
  Sources/CodexBarCore/Providers/OpenRouter/OpenRouterSettingsReader.swift \
  Sources/CodexBarCore/Providers/Codebuff/CodebuffSettingsReader.swift \
  Sources/CodexBarCore/Providers/Groq/GroqSettingsReader.swift \
  Sources/CodexBarCore/Providers/ElevenLabs/ElevenLabsSettingsReader.swift \
  -o /tmp/codexbar_endpoint_override_smoke
$ /tmp/codexbar_endpoint_override_smoke
endpoint override smoke passed: host:port bare overrides normalize to HTTPS; explicit HTTP rejects

$ python3 - <<'PY'
from pathlib import Path
files = [
    'Sources/CodexBarCore/Providers/OpenRouter/OpenRouterSettingsReader.swift',
    'Sources/CodexBarCore/Providers/Codebuff/CodebuffSettingsReader.swift',
    'Sources/CodexBarCore/Providers/Groq/GroqSettingsReader.swift',
    'Sources/CodexBarCore/Providers/ElevenLabs/ElevenLabsSettingsReader.swift',
    'Tests/CodexBarTests/ProviderEndpointOverrideSecurityTests.swift',
]
for f in files:
    for i, line in enumerate(Path(f).read_text().splitlines(), 1):
        if len(line) > 120:
            raise SystemExit(f'{f}:{i}: line length {len(line)}')
print('line length <= 120 for endpoint override files')
PY
line length <= 120 for endpoint override files

$ DYLD_FRAMEWORK_PATH=/Library/Developer/CommandLineTools/usr/lib \
  .build/lint-tools/bin/swiftlint --strict --config .swiftlint.yml \
  Tests/CodexBarTests/ProviderEndpointOverrideSecurityTests.swift \
  Sources/CodexBarCore/Providers/OpenRouter/OpenRouterSettingsReader.swift \
  Sources/CodexBarCore/Providers/Codebuff/CodebuffSettingsReader.swift \
  Sources/CodexBarCore/Providers/Groq/GroqSettingsReader.swift \
  Sources/CodexBarCore/Providers/ElevenLabs/ElevenLabsSettingsReader.swift
Done linting! Found 0 violations, 0 serious in 5 files.

$ swift test --filter ProviderEndpointOverrideSecurityTests
/private/tmp/codexbar-main-security-audit/.build/checkouts/KeyboardShortcuts/Sources/KeyboardShortcuts/Recorder.swift:172:1: error: external macro implementation type 'PreviewsMacros.SwiftUIView' could not be found for macro 'Preview(_:body:)'; plugin for module 'PreviewsMacros' not found

$ make check
SwiftFormat completed in 0.16s.
0/998 files require formatting.
SourceKittenFramework/library_wrapper.swift:58: Fatal error: Loading sourcekitdInProc.framework/Versions/A/sourcekitdInProc failed

The smoke script asserts:

  • localhost:8080 and localhost:8080/v1 are treated as bare host/path overrides and normalize to HTTPS.
  • http://attacker.test... remains rejected by the validation path for OpenRouter, Codebuff, Groq, and ElevenLabs.

Disclosure notes

@clawsweeper

clawsweeper Bot commented Jun 1, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed June 11, 2026, 10:52 AM ET / 14:52 UTC.

Summary
Review failed before ClawSweeper could summarize the requested change.

Reproducibility: unclear. The review failed before ClawSweeper could establish a reproduction path.

Review metrics: none identified.

Merge readiness
Overall: 🌊 off-meta tidepool
Proof: 🌊 off-meta tidepool
Patch quality: 🌊 off-meta tidepool
Result: rating does not apply to this item.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Risk before merge

  • [P1] No close action taken because the review did not complete.

Maintainer options:

  1. Decide the mitigation before merge
    Retry the Codex review after fixing the execution failure.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] Review did not complete, so no work-lane recommendation was made.
Review details

Best possible solution:

Retry the Codex review after fixing the execution failure.

Do we have a high-confidence way to reproduce the issue?

Unclear. The review failed before ClawSweeper could establish a reproduction path.

Is this the best way to solve the issue?

Unclear. Retry the review first so ClawSweeper can evaluate the actual issue and fix direction.

AGENTS.md: unclear because the file could not be read completely.

Codex review notes: model internal, reasoning high; reviewed against bea2a84e862f.

Label changes

Label changes:

  • add rating: 🌊 off-meta tidepool: Overall readiness is 🌊 off-meta tidepool; proof is 🌊 off-meta tidepool and patch quality is 🌊 off-meta tidepool.
  • remove P2: Current review triage priority is none.
  • remove merge-risk: 🚨 compatibility: Current PR review selected no merge-risk labels.
  • remove proof: sufficient: Current real behavior proof status is not_applicable, not sufficient.
  • remove rating: 🦞 diamond lobster: Current PR rating is rating: 🌊 off-meta tidepool, so this older rating label is no longer current.
  • remove status: 👀 ready for maintainer look: Current PR status no longer selects a status label.

Label justifications:

  • rating: 🌊 off-meta tidepool: Overall readiness is 🌊 off-meta tidepool; proof is 🌊 off-meta tidepool and patch quality is 🌊 off-meta tidepool.
Evidence reviewed

What I checked:

  • failure reason: retryable codex transport failure.
  • codex failure detail: Codex review failed for this PR with exit 1.
  • codex stderr: URL(.
  • codex stdout: No stdout captured.

Likely related people:

  • unknown: Codex failed before it could trace repository history. (role: review did not complete; confidence: low)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 1, 2026
@Hinotoi-agent

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review please — updated the branch at f13d1e9a to address the host:port bare override parsing and strict-lint feedback.

I also updated the PR body with copied redacted after-fix output, including:

  • swift build --target CodexBarCore completing successfully.
  • Focused Swift smoke output: endpoint override smoke passed: host:port bare overrides normalize to HTTPS; explicit HTTP rejects.
  • Line-length check output for the touched endpoint override files/tests.
  • Direct strict SwiftLint output showing Done linting! Found 0 violations, 0 serious in 5 files.

The local swift test --filter ProviderEndpointOverrideSecurityTests and full make check blockers are still documented separately in the PR body; they occur before the touched tests run / inside the wrapper SourceKit load path, while the focused after-fix behavior proof uses fake/redacted endpoint values only.

@clawsweeper

clawsweeper Bot commented Jun 1, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@steipete
steipete force-pushed the harden-sibling-provider-endpoint-overrides branch from f13d1e9 to 0b0ea3d Compare June 11, 2026 14:14
@steipete

Copy link
Copy Markdown
Owner

Rebased onto landed #1269 and removed the duplicate stacked validator commits. Exact head 0b0ea3db now contains only the sibling-provider extension and maintainer hardening.

Additional fixes:

  • reject raw and percent-encoded whitespace/control characters in sibling provider hosts
  • prove OpenRouter, Codebuff, Groq, and ElevenLabs fetchers reject insecure overrides before request construction

Verification:

  • swift test --filter ProviderEndpointOverrideSecurityTests: 6 passed
  • make check: clean
  • structured autoreview: clean, no actionable findings

@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jun 11, 2026
@steipete
steipete force-pushed the harden-sibling-provider-endpoint-overrides branch from 0b0ea3d to 80f861e Compare June 11, 2026 14:47
@steipete

Copy link
Copy Markdown
Owner

Rebased onto current main and force-pushed exact head 80f861e3.

Proof:

  • swift test --filter ProviderEndpointOverrideSecurityTests: 7 tests passed.
  • make check: passed.
  • Autoreview initially raised bracketed IPv6 compatibility; local Foundation behavior and the existing MiniMax regression showed the claim did not reproduce.
  • Added explicit OpenRouter, Codebuff, Groq, and ElevenLabs IPv6 regressions; final autoreview is clean with no accepted/actionable findings (confidence 0.86).

Fresh exact-head CI is required before merge.

@clawsweeper clawsweeper Bot added rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. and removed proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jun 11, 2026
@steipete
steipete merged commit 88c43ee into steipete:main Jun 11, 2026
4 checks passed
@steipete

Copy link
Copy Markdown
Owner

Validated exact head 80f861e3ec30799f880ea1de5fc8fde09c5640e5.
Landed as 88c43eeb8485f6f60b5a6b64de45e7757e051c3d.

  • focused endpoint suites (113 tests), including valid bracketed IPv6 preservation
  • swift test (3,621 tests)
  • make check
  • isolated CLI smoke tests for OpenRouter, Codebuff, Groq, and ElevenLabs with dummy credentials and http://127.0.0.1:9; every provider rejected the override before network I/O
  • malformed, credentialed, encoded-delimiter, whitespace, control-character, and host-port cases covered
  • autoreview clean
  • released/fictitious model-name gate clean
  • GitHub CI green on the exact head

Policy outcome: plaintext credentialed endpoint overrides are intentionally rejected; valid HTTPS proxies and bare hosts remain supported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants